-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Biasstep rebias #362
Biasstep rebias #362
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what I understand, the functionality is good, so my comments are more code hygiene things. There's some excess whitespace, long lines, and repeated sections of code that should be cleaned up. Doing so will help with readability and make debugging easier if needed.
sodetlib/operations/bias_dets.py
Outdated
Scripts work well when detectors are already in transistion. For extreme cases | ||
such as detectors are all in SC, it takes a few run to put them perfectly in | ||
transition. Next step is making this script can handle extreme cases faster | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Docstring should have purpose of the function and description of args/kwarrgs. This note is fine to put after those items.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also an overview of how the script works would be nice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
sodetlib/operations/bias_dets.py
Outdated
time.sleep(60) | ||
safe_dc_biases = previous_dc_biases.copy() | ||
for replace_bg in bg_overbias_needed: | ||
safe_dc_biases[replace_bg] = cfg.dev.bias_groups[replace_bg]["testbed_100mK_bias_voltage"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that a standard entry in the config file? In other words, is it guaranteed to exist?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is not a standard entry but should be added in.
the purpose for this entry is so the script has a reference of what bias_voltage to use such that the detectors won't be in SC state at the site. This can also be understood as the bias_voltages at the minimum loading.
since all modules are tested in Princeton first, the dark detector bias_voltages are naturally the bias_voltages at the minimum loading. If this info is missing for some reason, user can put in some high numbers such as 15V for Uv31 as a start point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you do some combination of (a) adding that parameter to det_config.py (b) using a .get()
call here to return a sensible default if no matching entry is found?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I second this. If you add it to the bg_defaults
it will always be loaded and you won't need the get
call though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a way in the current det_config.py such that we can load different numbers for 90s and 150s?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we name this something other than testbed bias voltage? I saw "testbed" and was very confused about why that was useful on-site until I saw you explanation here.
"dark_bias_voltage" or even "reference_bias_voltage" could make this clearer. And it would (correctly) imply that we could change these numbers later as we start to know the UFMs and on-sky loading better.
## add in check if detectors are normal now | ||
percentage_rn_0 = bsa_0.R0/bsa_0.R_n_IV | ||
drop_from_normal = False | ||
for bl in bg_overbias_needed: | ||
mask_bg = np.where(bsa_0.bgmap==bl)[0] | ||
per_bl_percentage_rn_0 = percentage_rn_0[mask_bg] | ||
## mask for normal | ||
mask_normal = np.where((per_bl_percentage_rn_0 > 0.9) | (per_bl_percentage_rn_0 < -0.9)) | ||
## if more than half of the detectors are normal | ||
if len(mask_normal[0]) > 0.5*len(mask_bg): | ||
bg_detectors_normal.append(bl) | ||
drop_from_normal = True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code block is repeated from before (I think). Could be tidied up to avoid repetition.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code block is not repeated. this code block is designed to be ran again if over_biasing is needed. After the overbiasing, the script has to choose some bias_voltage to land on, in order to be safe from dropping to SC again, the script will choose the biasvoltage recorded in the config file as "testbed_100mK_bias_voltage", this however might land the detectors in normal state. Hence this code block is for catching this after the overbiasing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I'm trying to point out is that
sodetlib/sodetlib/operations/bias_dets.py
Lines 325 to 332 in 05fea5d
mask_bg = np.where(bsa_0.bgmap==bl)[0] | |
per_bl_percentage_rn_0 = percentage_rn_0[mask_bg] | |
## mask for normal | |
mask_normal = np.where((per_bl_percentage_rn_0 > 0.9) | (per_bl_percentage_rn_0 < -0.9)) | |
## if more than half of the detectors are normal | |
if len(mask_normal[0]) > 0.5*len(mask_bg): | |
bg_detectors_normal.append(bl) | |
drop_from_normal = True |
sodetlib/sodetlib/operations/bias_dets.py
Lines 289 to 296 in 05fea5d
mask_bg = np.where(bsa_0.bgmap==bl)[0] | |
per_bl_percentage_rn_0 = percentage_rn_0[mask_bg] | |
## mask for normal | |
mask_normal = np.where((per_bl_percentage_rn_0 > 0.9) | (per_bl_percentage_rn_0 < -0.9)) | |
## if more than half of the detectors are normal | |
if len(mask_normal[0]) > 0.5*len(mask_bg): | |
bg_detectors_normal.append(bl) | |
drop_from_normal = True |
so it could be, for example, condensed into a helper function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh yeah yeah good point... but I am hoping it is ok to just leave it like this as it just used only twice
Hi Yuhan, I'm about to start reviewing this, but do you think you can post here (or in the docstring) like the overall strategy for how this works? Something like a flow-chart that documents what it will try to do at each step under what conditions, etc. would be really helpful for understanding this at a higher level |
the main logic is described in 4.2.2 of extra parts of this script is for handling failures of bias_step and handling extreme situations of detectors stucked in SC state or in normal state |
9f86262
to
05fea5d
Compare
sodetlib/operations/bias_dets.py
Outdated
if np.isnan(np.nanmedian(per_bl_R0)): | ||
## the yield is very low, could it be the biasstep analysis script failed? repeat again | ||
repeat_biasstep = True | ||
S.log("the biasstep script might have failed, going to repeat the measurement") | ||
|
||
if repeat_biasstep: | ||
S.log("retaking biasstep due to failure") | ||
bsa_0 = bias_steps.take_bias_steps(S, cfg) | ||
repeat_biasstep = False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really like that this is forcing a retake of the bias steps, especially since if it fails a second time it will just continue on as if everything's ok. I think instead if the first bias steps fails, this should raise an exception with a useful error message so a user can take control and figure out the issue.
Do you know how often this fails / why? We should try to fix in the bias step function directly instead of building fixes into this function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bias step failure has been noticed in multiple places including at the site in LATR, in Chicago LATRt, TSAT and Cornell. One of the failure mode might due to the if statement here:
sodetlib/sodetlib/operations/bias_steps.py
Line 645 in 4aafb94
if not transition: |
Tho repeating the biasstep is not ideal, it is a working method given the current circumstances.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see how repeating it changes anything though... If we want, we can pass an arg that forces it to use the transition
estimation of resistance if we think it should be in the transition state. also I believe Yaqiong is also working on a better way to determine whether a channel is in transition.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but simply repeating it worked. yes Yaqiong and I have been reviewing some functions together.In my opinion I think the if statement can just be removed as we don't really rely on a accurate tes resistance from the biasstep when TES is not in-transition.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think it's mostly failing when a large fraction of detectors are superconducting? I notice you don't have retries for other times you call bias steps.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added in a retry for each bias step except the last one I think. I think the failing can happen either when a large fraction of detectors are superconducting or being normal.
also repeating biasstep twice is like a old voodoo from my ACT days. I think such fix might make sense if one imagines some ADC needs to be 'waken'. I am not sure but I agree this can be redundant and can be removed after better understanding later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
awesome, that's really interesting. are these files all on simons1? I'll take a look tomorrow.
sodetlib/operations/bias_dets.py
Outdated
for sleep_index in range(5): | ||
time.sleep(60) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the sleep-time after overbiasing should be a configurable parameter, ideally settable in the dev-cfg
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah good idea.
…is used in the biasstep_rebias script
except KeyError: | ||
mask_bg = np.where(bsa_0.bgmap==replace_bg)[0] | ||
v_norm_bl = np.nanmedian(iva.v_bias[iva.idxs[[mask_bg], 1]]) | ||
safe_dc_biases[replace_bg] = v_norm_bl | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't need this except anymore since it's in the default dev cfg dict.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i agree.
speaking of this, what should be the correct way of generating a config file. I am not sure generating from the default dev cfg dict is practiced a lot currently in different places
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In your sysconfig, if you set the devcfg path as one that doesn't exist, it will try to create a new one there with the defaults
sodetlib/operations/bias_dets.py
Outdated
while drop_from_normal: | ||
S.log(f"some biaslines are still in normal state: {bg_detectors_normal}") | ||
percentage_rn_0 = bsa_0.R0/bsa_0.R_n_IV | ||
S.log("droping detectors from normal to transition") | ||
previous_dc_biases_dfn = S.get_tes_bias_bipolar_array() | ||
|
||
S.log(f"current tes bias voltages are: {previous_dc_biases_dfn}") | ||
for bl in bg_detectors_normal: | ||
mask_bg = np.where(bsa_0.bgmap==bl)[0] | ||
v_sc = iva.v_bias[iva.idxs[[mask_bg], 0]] | ||
v_norm = iva.v_bias[iva.idxs[[mask_bg], 1]] | ||
v_spread = np.nanmedian(v_norm) - np.nanmedian(v_sc) | ||
if previous_dc_biases_dfn[bl] > v_spread: | ||
previous_dc_biases_dfn[bl] = previous_dc_biases_dfn[bl] - 0.5 * v_spread | ||
else: | ||
previous_dc_biases_dfn[bl] = previous_dc_biases_dfn[bl] - 0.3 * v_spread | ||
bg_detectors_normal.remove(bl) | ||
S.log(f'biasline {bl} is approching 0 voltage, a confirmation new IV curve is recommended') | ||
|
||
S.set_tes_bias_bipolar_array(previous_dc_biases_dfn) | ||
S.log(f"applying: {previous_dc_biases_dfn}") | ||
S.log(f"waiting 10s for dissipation of UFM local heating") | ||
time.sleep(10) | ||
bsa_0 = bias_steps.take_bias_steps(S, cfg) | ||
percentage_rn_0 = bsa_0.R0/bsa_0.R_n_IV | ||
drop_from_normal = False | ||
for bl in bg_detectors_normal: | ||
mask_bg = np.where(bsa_0.bgmap==bl)[0] | ||
per_bl_percentage_rn_0 = percentage_rn_0[mask_bg] | ||
## mask for normal | ||
mask_normal = np.where((per_bl_percentage_rn_0 > 0.9) | (per_bl_percentage_rn_0 < -0.9)) | ||
## if more than half of the detectors are normal | ||
if len(mask_normal[0]) > 0.5*len(mask_bg): | ||
drop_from_normal = True | ||
else: | ||
bg_detectors_normal.remove(bl) | ||
for bl in bg_detectors_normal: | ||
if previous_dc_biases_dfn[bl] < 0.5: | ||
bg_detectors_normal.remove(bl) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain a bit about what this is doing? I don't quite understand how you're chosing the bias voltage step sizes, and also it's not clear to me from the code how many times this runs on average, but seems like it could be a lot of bias step measurements. Do you think this is mainly what's dominating the function's runtime?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we might want to brainstorm other ways to drop into the transition. I think if we're taking small enough step sizes we may not need to take bias steps every time, since we're unlikely to induce tracking skips.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is looping down the bias voltages until a certain bias line is no longer in normal state and TES start to be in-transtion. The step size being the 50% of the transition region from the latest IV such that the bias_line won't be dropped into SC due to step being too large or the script takes too long due to the step being to small. Notice the latest IV doesn't necessary provides good estimation as the I-V won't be taken as frequently as the bias-step, so a larger step size here can be dangerous.
in the field I think the function runtime is dominated by the bias-step analysis itself. This is because in the field the sky loading changes gradually and slowly. the longest runtime will be the wait time of biasing detectors out of SC state, but this shouldn't happen if the re-biasing is done frequently enough nor should dropping from normal state happen frequently.
sodetlib/operations/bias_dets.py
Outdated
S.log(f"applying: {previous_dc_biases_dfn}") | ||
S.log(f"waiting 10s for dissipation of UFM local heating") | ||
time.sleep(10) | ||
bsa_0 = bias_steps.take_bias_steps(S, cfg) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you do not need to run on all bias lines, you can specify the bgs
arg here to just run on a selection of them. This may speed up the function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a good idea, but I think this should be added into the next generation of this code. This is because I currently rely on bg map for all 12 biaslines in this version of code.
Hi Yuhan, finally was able to read through the entire function. Here are the notes I took about the control flow. Do you agree with this as the intended behavior?
|
Can you give an estimate of the run-time? |
4 mins if start from in-transition, 14 mins if start from SC. (tested in LATRt) |
I like this description of the flow of the function. Since @yuhanwyhan gave the thumbs up to the description can you commit that to the docs rst file along with a link to Yuhan's conference proceedings @jlashner ? |
there is one potential rare failure mode I just realized. the solution for this issue will require some re-writing. the current script forces taking bg map for all 12 biaslines because when it calculates the v_estimate, it does the calculation for all biaslines and then apply the bg_map. One possible fix will be adding a for loop looping over the chosen biaslines and apply the bg_map inside the for loop such that the script only calculate the v_estimate for the chosen biaslines. I suggest adding this fix into the later version of this script when we can test it cold again. Right now without a cold cryostat and MF/UHF UFMs, I am not confident that this change won't break the code. |
a way to make the script faster will be only running the R_tes calculation from biasstep except the final biasstep (running only the _compute_R0_I0_Pj part). An estimation of the run-time for each analysis block of the current biasstep function will be nice. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this has been tested, I'm fine with merging this now with minimal changes to the code. I think we can merge as long as we make the following updates to the documentation:
- Add a link to the spie proceeding describing the procedure in the docstring
- Add my step-by-step overview of what the function is doing to the docstring (or a better overview if someone else wants to write one)
- Add a comment or some sort of documentation describing how
testbed_100mK_bias_voltage
is being used. I'm not exactly sure how this is used right now, but think we may want to change the name a bit, since this will be used in non-testbed systems, and I think this should probably have the word "transition" in it somewhere. Maybe100mK_transition_bias_voltage
?
As we test this more and start to use it in the field, I think we will probably want to clean up / update the functionality, but that can all go into separate PRs.
@@ -201,3 +201,408 @@ def bias_to_rfrac(S, cfg, rfrac=0.5, bias_groups=None, iva=None, | |||
|
|||
return biases | |||
|
|||
def biasstep_rebias( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we set this function as an action? We will want this to be categorized as a single detector operation instead of "many many" bias steps in the data packaging. We'll also want to change the default tags getting added to these .g3 files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a simple docs page linking to the spie paper, and made the function a pysmurf action. I think I am happy to merge this in now and solve any problems with it as they come up.
@kmharrington mentioned 2 additional things needed for this to be used within the site data operations, and packaging:
I’m happy to wait on 2 until later but if 1 is a simple-ish modification I would like to get that in before merge. We want to use this soon in SAT-MF1 though so if not simple ok to merge and open new issue/or with these additional things. |
Right, thanks for reminding me. Latest commit should do that, and sets all g3_tags to |
Thanks let's get this merged then |
this contains the script of re-biasing detectors using biasstep.
this function has recently been tested in LATR-t with different loading conditions.
this plot is showing the result of re-biasing detectors when LATR-t ful exposed to the room
this plot is showing the result of re-biasing detectors after covering the LATR-t with a metal plate
Notice the tests were done with UHF detectors (Uv31), and the change of loading between these two test are very large.
in the field we expect lower and more gradually change of loadings.
The next level thing is to make the script run faster, this might involve only running part of the analysis code for bias-step.